-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override default dashboard background with theming one #23588
Override default dashboard background with theming one #23588
Conversation
Right, so the idea would be that theming has a setting called "Use theming background for Dashboard" and it is then forced and non-changeable by users? Or is it just the default one in that case? (I’d prefer the latter.) |
Could you possibly add a "no background" (as in, theme colour) option? That would just consoldate your patch and provide a config that covers all the cases a user themself could also pick from the GUI. |
@jancborchardt Yes it would just replace the default one, still letting users change it as usual.
@lazerl0rd Do you mean make the "plain background" as default? Yes it would be nice indeed. |
Yupp, that's exactly what I meant. |
Ready for reviews? |
@skjnldsv Still WIP. Dashboard side is ready but additional settings in theming app are still missing. What do you think about the idea of using appdata to store the image set in theming app? Any other/better idea? |
labels and/or draft please :)
I'm not opposed to it. |
a990f85
to
282bcf7
Compare
@skjnldsv Sorry 😅 So here it is after getting lost in the theming css maze 😁. Dashboard theming setting behaves just like the login image one. Delete button switches to plain color, revert button switches to the clouds image, upload button let admins choose a custom default dashboard background. If dashboard app is disabled, related theming settings are hidden. Incorrect Theming app could be Vuetified ™️ but that's another story. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 🐘
if (background === 'default') { | ||
if (themingDefaultBackground && themingDefaultBackground !== 'backgroundColor') { | ||
return generateUrl('/apps/theming/image/dashboardBackground') + '?v=' + time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return generateUrl('/apps/theming/image/dashboardBackground') + '?v=' + time | |
return generateUrl('/apps/theming/image/dashboardBackground') + '?v=' + window.OCA.Theming.cacheBuster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing you mentioned that, there is a mistake at
'cacheBuster' => $this->appConfig->getAppValue(Application::class, 'cachebuster', '0'), |
Fixed in this branch. It will most likely solve many late favicon/icon/image refresh problems. Yay!
Code looks good but I'm a bit hesitant to add yet another setting to the theming app. This is quite a special use case imo and I'd rather just reuse the themed login image and not allow another configuration option here. But up to @jancborchardt to decide. |
Thanks for the reviews!
I agree we can't add too much stuff there. I still like the possibility to set a different default background than the login one. It would feel a bit less crowded if login and dashboard sections would be side by side like that: @jancborchardt @juliushaertl What do you think? |
Totally agree with @juliushaertl here – especially since there is already the ability to individually select the ones we ship as default Dashboard backgrounds, so it makes sense to just unceremoniously replace the "Default" Dashboard background with the one set by theming. :) (This would btw also help for customers, which gives a unified style by default, and based on that people can customize.) |
2c8b952
to
9943855
Compare
@juliushaertl @jancborchardt Right, new behaviour is: If theming app is disabled or the login image is not set, no effect on the dashboard. Otherwise the dashboard default background is the same as set in theming (image or plain). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Needs a rebase 😉 |
9943855
to
c84f451
Compare
/compile amend / |
fix getAppValue default value in theming app fix cacheBuster value injection Signed-off-by: Julien Veyssier <[email protected]> Signed-off-by: npmbuildbot-nextcloud[bot] <npmbuildbot-nextcloud[bot]@users.noreply.github.com>
c84f451
to
f5ef2d7
Compare
Thanks for the reviews. |
/backport to stable20 |
Currently, dashboard does not look good if theming is enabled and set to a light background. See #28649. Is there a way to have dark text color and dark icons for light backgrounds? |
Why can't dashboard just use the color scheme for the application bar that every app uses? |
Too bad that Dashboard even overrides the settings from custom-css-App. |
Please open new issues to discuss such things, commenting on one year old pull request will not help here. |
If the override switch is on, the dashboard will look for a default background in "default" directory of dashboard's appdata.
This way the theming app just has to put an image there and toggle the config switch (it's not done yet).
@juliushaertl @jancborchardt Do you think it's a good way to do that?